Refactor v2 #6
Refactor v2 #6
Conversation
Forward port #3
This proposal is a simple implementation of ContainerInterface. Now we can use it in [zend-stratigility-skeleton](https://github.com/container-interop/container-interop) for example.
|
@bakura10 I'm finishing up the Apigility 1.2 release in the next day or so, and then will give this another thorough review. I'm torn on the Thanks for all your hard work on driving this forward! |
|
Thanks! Just to know, I'm trying to run the tests locally (and they actually fail in Travis for the same reason) because you told me to remove the bootstrap file. How are we supposed to do it now? |
|
Modify the phpunit.xml file to point to the vendor/autoload.php file for
|
|
Hi, I've updated and fixed the tests. I'm a bit lost with all the travis tasks that are run, it seems there is a bit if inconsistency between various ZF components. Couldn't we have a simple template on which tasks should run? |
|
Nice work @bakura10. I'd started down this same road because I needed options on main SM; looks like you hit the nail on the head, our code wasn't too different - I'd serialized and hashed options in the cache to return same-options-configuration from cache (e.g., SMTP mailers). I came here to I hope this gets pulled in quickly! Glad aliases are still in. |
|
Note for myself: in the configure method we must do an array_merge to allow a local configuration that is merged with the configuration given in constructor (mostly needed for plugin managers) |
|
Hi @weierophinney, I've mode some final changes, for me the code is nearly finished and could be reviewed :). Some parts are still untested (relative to delegators). If someone want to give it a try, do not hesitate to do a PR to my branch. |
| */ | ||
| public function createService(ServiceLocatorInterface $serviceLocator) | ||
| public function __invoke(ServiceLocatorInterface $serviceLocator, $requestedName, array $options = []) | ||
| { | ||
| $config = $serviceLocator->get('Config'); |
There was a problem hiding this comment.
@weierophinney , I'm still unsure how this is supposed to work now that Service Manager is a standalone component... If someone is using this component without the context of the ZF2 app, this will simply fail.
This gives yet another point for @Maks3w approach to really have one standalone component that can be used for non ZF user (zend-servicemanager) AND an additional module to be consumed by ZF user (zend-servicemanager-module) whose only task will be to integrate with the module manager and provide its config.
This will really going to be a huge challenge to make everything work nicely now that we cannot make any assumption on what is installed or not.
|
I started to hate this kind of PR where the diff is fully of dirty changes. Please @bakura10 Review your own PR file by file, line by line and make a PR clean and focused only in the changes related to the PR. Also add a resume in the PR description of the changes you made, why are needed, etc. |
|
This PR is cleaned and nearly finished. Once again it's a complete rewrite of the component, I'm not sure how I can do that better. I could do like for validator by doing a first PR that removes everything, then another PR on my branch with only the changes? But I'd prefer if review could be donehere. |
|
@bakura10 Your PR basically only should have changes in src and test directory. Rest of changes are not related and even are broken. Again, REVIEW your own PR |
|
What do you mean by review my own PR ? I have reveiwed it and I have nothing else to say about it. Why other changes are broken? Changes made to composer.json are part of the PR. |
|
@bakura10 Do you find normal to change the LICENSE? |
|
Oops, definitely not. I'm not sure why it has been converted to .txt file actually :(. |
|
@bakura10 That is not the only file. Please last call open https://github.com/zendframework/zend-servicemanager/pull/6/files and clean the changes |
It's hard to review your PR. Too much noise in your code changes. (Separation of concerns / separation of changes is missing in your PR. 😉) |
|
I think the diff problem comes from the fact that this PR contains unrelated commits (see the first commits). I guess it's because of a wrong rebase. And I'm afraid that by fixing each of those changes manually in new commits you are only adding more changes. If you rebase against master properly (and squash/rename some of your commits) it should solve the problems. |
|
Ah or maybe it's because this PR is against master and not develop? |
|
see the first commit of the author and your questions will be a answered |
|
Since this is a development step to version 3 this should be merged to develop and not to master. I think that was the last comment of @weierophinney in #2 |
|
Yeah, I've been trying agian but the issue is that deleted files still appear in the PR and I'm unable to hide them :/. @Maks3w is currently offline but if anyone knows how to do that I4m on IRC right now (#zftalk.dev) :D |
|
First thing might be to recreate the pull request again develop (since this branch seems to be based on develop). |
|
I've been able to do it against my branch: bakura10#6 |
|
Aren't you on IRC ? :) |
|
Close it in favour of #8 |
|
Not sure if this helps, but sometimes when I run into such issues, I create fresh branch, import all changes from "corrupted" one:
and then I use |
|
you can do |
| * @param object $instance | ||
| * @return void | ||
| */ | ||
| public function __invoke(ServiceLocatorInterface $serviceLocator, $instance); |
There was a problem hiding this comment.
What is a state of this iterface? This thread looks interesting zendframework/zendframework#6068
There was a problem hiding this comment.
For BC purposes, we will likely continue to need this, though we may want to mark it as deprecated.
While delegator factories are indeed an awesome feature, they also are opt-in, and in many cases — specifically with the EM — developers are currently trained to just expect the instance to be injected. I cannot think of a migration plan that would ease that transition currently.
There was a problem hiding this comment.
I like idea of initializers but not as default in zf. Initializers are great feature to use in plugin manager where we can use methods from validated interfaces
There was a problem hiding this comment.
I'm not sure to understand what you mean by "initializers are great feature to use in plugin manager where we can use methods from validated interfaces". Can you please clarify?
|
@snapshotpl , please review #8 instead, this is the last one. |
…gets-applied Apply PR zendframework#242 to zend-master
Hi,
This replace the other PR as I suck at rebasing.
What do we need next for the PR @weierophinney ? you didn't give us your opinion regarding the options stuff. This is the only important thing that I'm unsure yet.